WIP: sfdisk implementation by prestist · Pull Request #1929 · coreos/ignition

prestist · 2024-08-26T21:29:14Z

This is a WIP PR to add sfdisk support to ignition, Current state is iterating through failing tests and adding mapping to pretend to interface the same as before with sgdisk.

Resolves #1926

prestist · 2024-09-03T13:20:21Z

I think I might need a bit more for wipefs to work correctly. (looking into it but wanted to share)

Clean device using old way

[core@cosa-devsh ~]$ sudo sgdisk --zap-all /dev/vda
Found valid GPT with corrupt MBR; using GPT and will write new
protective MBR on save.
GPT data structures destroyed! You may now partition the disk using fdisk or
other utilities.

Write to device using command I will use again to verify it works

[core@cosa-devsh ~]$ sudo sgdisk --new=1:0:+65536 --change-name=1:create-partition --typecode=1:B921B045-1DF0-41C3-AF44-4C6F280D3FAE --partition-guid=1:05AE8178-224E-4744-862A-4F4B042662D0 /dev/vda
Creating new GPT entries in memory.
The operation has completed successfully.

Wipe device using wipefs to see if we can use a device that is wiped with it

[core@cosa-devsh ~]$ sudo wipefs -a -t mbr -t gpt -t pmbr /dev/vda
/dev/vda: 2 bytes were erased at offset 0x000001fe (PMBR): 55 aa
/dev/vda: calling ioctl to re-read partition table: Success

Verify disk is empty according to sfdisk

[core@cosa-devsh ~]$ sudo sfdisk --list /dev/vda
Disk /dev/vda: 2 GiB, 2147483648 bytes, 4194304 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes

Try sgdisk command to create partition created above.

[core@cosa-devsh ~]$ sudo sgdisk --new=1:0:+65536 --change-name=1:create-partition --typecode=1:B921B045-1DF0-41C3-AF44-4C6F280D3FAE --partition-guid=1:05AE8178-224E-4744-862A-4F4B042662D0 /dev/vda
Found valid GPT with corrupt MBR; using GPT and will write new
protective MBR on save.
Could not create partition 1 from 67584 to 133119
Error encountered; not saving changes.

prestist · 2024-09-04T21:43:30Z

Hmm, I seem to be stumbling a bit, I ended up focusing directly on the pretend section today. I found a few odds and ends that were missed in the first pass.

I had been running against a creation test which only created one partition, however I stumbled into a test which makes two partitions, this is relevant because the nature of how the sfdisk is currently receiving my script input is painful. Currently I don't see a way to express multiple partitions when running commands in the echo <my script> | sudo sfdisk /dev/vda

using '\n' does not do what I would hope, so I think that leaves me two two imput ideas which I see

use cmd.stdin() after running cmd.execute(sfdisk /dev/vda)
create a file and read it in sfdisk /dev/vda < myfile.txt

I have been leaning towards option 1 as it seems the least invasive, but I dont think its working as I expect, and its difficult to debug blackbox tests.

Is option 2 even acceptable?

More on option 1:

Given ignition config 👇

            {
                        "ignition": {
                                "version": "3.3.0"
                        },
                        "storage": {
                                "disks": [{
                                        "device": "/dev/loop22",
                                        "partitions": [
                                        {
                                                "label": "newpart",
                                                "sizeMiB": 32
                                        },
                                        {
                                                "number": 1,
                                                "label": "foobar"
                                        }
                                        ]
                                }]
                        }
                }

the code expands it into an array of commands for sfdisk

{ "1 : start=2048 size=65536 name=foobar " ,
"size=65536 name=newpart "}

	cmd := exec.Command("sh", "-c", fmt.Sprintf("sudo %s %s", distro.SfdiskCmd(), op.dev))
	for _, s := range scripts {
		println("script %s", s)
		cmd.Stdin = bytes.NewBufferString(s)
	}
	cmd.Stdin = bytes.NewBufferString("quit")
	cmd.Stdin = bytes.NewBufferString("Y")

However then subsequently running a dump command I only find this std output

label: gpt
label-id: E800E3D5-7AE6-46ED-AC03-AD8E17F81947
device: /run/ignition/dev_aliases/dev/loop22
unit: sectors
first-lba: 34
last-lba: 133119
sector-size: 512

Which does not have any partitions listed, acting out these commands manually does; I wonder if cmd.stdin works like I am using it.

prestist · 2024-09-05T20:10:33Z

Ok, in the latest push a few things have happened.

Pulled in some code from sgdisk into sfdisk to allow for easier testing aka sgdiskcommit() and sgdiskbuildoptions()
Focused on pretend, it looks like sfdisk is now pretending correctly based on the scripts provided, which is good, as I was stuck. The answer was "%s" my script so the new line characters would work properly.
Found out that the code added for wipe is not sufficient, as it results in a currupt partition table. Spoke to @jlebon and I am going to look into how --zap-all works with sgdisk.

Next steps:

Found that partitions is doing some interpreting based on the pretend and it has an understanding of sgdisk today, need to expand that for sfdisk.
Proceed into commit
Fix wipe, as sgdisk --zap-all is functional and the code in sfdisk using wipefs results in a corrupt partition table.

jlebon · 2024-09-12T19:40:12Z

internal/sfdisk/sfdisk.go

+	// If wipe we need to reset the partition table
+	if op.wipe {
+		// sgdisk --zap-all /dev/sda destroys the partition table, let's do the same with wipefs
+		// Found out that wipefs is not sufficient, as it results in a corrupt partition table, look into sgdisk's way of doing --zap-all


Looks like util-linux/util-linux#3191 is an outstanding RFE to have an sgdisk --zap-all like option for sfdisk. We should verify the sgdisk code, but according to util-linux/util-linux#3191 (comment), it should be sufficient to zero out the first and last 34 sectors of the disk.

Hmm but actually, testing locally I can't get the corrupted partition table message to happen after using wipefs -a. It also seems like wipefs -a does not delete filesystem data hosted at the partitions as I previously thought.

Thank you; Yeah -a seems like the right combo for our needs. Thank you thank you.

internal/sfdisk/sfdisk.go

prestist · 2024-09-16T20:46:43Z

Latest push includes the parsing for sfdisk's pretend. Additionally added a few unit tests to sanity check the parsing. Will likely need to expand with examples but so far thats passing.

Additionally adjusted the wipe script as per @jlebon 's comment.

Next steps (from last comment):

✅ Found that partitions is doing some interpreting based on the pretend and it has an understanding of sgdisk today, need to expand that for sfdisk.
☑️ Proceed into commit
✅ Fix wipe, as sgdisk --zap-all is functional and the code in sfdisk using wipefs results in a corrupt partition table.

prestist · 2024-09-17T21:09:04Z

internal/exec/stages/disks/partitions.go

-)
+func getDeviceManager(logger *log.Logger, dev string) device_managers.DeviceManager {
+	// To be replaced with build tag support or something similar.
+	if false {


Note: will be dynamic based on arguments later just hard coded switch atm.

prestist · 2024-09-17T21:11:31Z

Update: added interface for device_managers (name could be better not sure what would be good; feedback wanted lol), thus abstracting implementation from partitions.go, additionally lifted and placed parse responsibility on the implementation rather then the caller.

Still need to work against commit.

jlebon · 2024-09-24T19:12:58Z

internal/device_managers/deviceManagers.go

@@ -0,0 +1,28 @@
+package device_managers


How about partitioners?

Yeah thats a better name; will do!

I know renames can be confusing in prs, so i did some commit juggling and it has been renamed; I hope I did it in a way that made sense.

internal/device_managers/sfdisk/sfdisk.go

jlebon · 2024-09-24T19:20:20Z

internal/device_managers/sfdisk/sfdisk.go

+
+	op.logger.Info("running sfdisk with script: %v", script)
+	exec.Command("sh", "-c", fmt.Sprintf("echo label: gpt | sudo %s %s", distro.SfdiskCmd(), op.dev))
+	cmd := exec.Command("sh", "-c", fmt.Sprintf("echo \"%s\" | sudo %s %s", script, distro.SfdiskCmd(), op.dev))


Same comments here.

Probably cleanest is to make a helper function that actually calls sfdisk and takes the script as an argument that it feeds to its stdin?

I gave it a swing

jlebon · 2024-09-24T19:30:56Z

internal/device_managers/sfdisk/sfdisk.go

+// ParseOutput takes the output from sfdisk. Similarly to sgdisk
+// it then uses regex to parse the output into understood values like 'start' 'size' and attempts
+// to catch any failures and wrap them to return to the caller.
+func (op *Operation) ParseOutput(sfdiskOutput string, partitionNumbers []int) (map[int]device_managers.Output, error) {


OK yeah, this is pretty unfortunate. I was expecting --json or -d to also work in combination with --no-act, but that doesn't seem the case. Let's at least file an RFE upstream for now?

This has a high chance of regression if the output of sfdisk changes. We'll indeed want to make sure we have good coverage for this.

Yeah; 100%, Ok will do and tag it here once done.

jlebon · 2024-09-24T19:32:04Z

internal/device_managers/sfdisk/sfdisk.go

+	}
+
+	// Prepare the data, and a regex for matching on partitions
+	partitionRegex := regexp.MustCompile(`^/dev/\S+\s+\S*\s+(\d+)\s+(\d+)\s+\d+\s+\S+\s+\S+\s+\S+.*$`)


This deserves some explanation.

Also, I feel like we should ignore anything before the "New situation:" line.

Added some details

prestist · 2024-10-08T21:55:08Z

Ugh; I accidentally rebased too far back and effected external commits sigh; I will fix that.

edit: fixed

prestist · 2025-09-10T13:08:46Z

Ok I pushed latest hacked changes including "failed-1.txt" and "sfdisk-failures.md" both files are intended to be removed but are being pushed to try and share the current state of the changes.

"failed-1.txt" is the output from failing blackbox tests. While "sfdisk-failures" is a summary of the failures and reasons why they might be happening from the perspective of an AI tool.

This is a copy of sgdisk, with mapping around sfdisk tooling. Update the buildOptions to understand sfdisk requirements and focus on adding support for the pretend function. Fixes: https://issues.redhat.com/browse/COS-2837

Add an interface to abstract implementation details for partition management, allowing tooling to pivot between implementations. TBD: argument/tag reading for triggering the pivot Fixes: https://issues.redhat.com/browse/COS-2930

alicefr · 2026-02-06T13:56:31Z

internal/exec/stages/disks/partitions.go

+			part.TypeGUID = &info.TypeGUID
+			part.GUID = &info.GUID
+			part.Label = &info.Label


Don't we need to check if the user has set those value in ignition, and if yes it shoud overwrite the existing value?

Suggested change

part.TypeGUID = &info.TypeGUID

part.GUID = &info.GUID

part.Label = &info.Label

if !cutil.NotEmpty(part.GUID) {

part.GUID = &info.GUID

}

prestist force-pushed the sgdisk-fdisk branch from 4ce4905 to dd8f0b9 Compare August 30, 2024 15:07

prestist force-pushed the sgdisk-fdisk branch from dd8f0b9 to bba6095 Compare September 5, 2024 20:03

jlebon reviewed Sep 12, 2024

View reviewed changes

internal/sfdisk/sfdisk.go Outdated Show resolved Hide resolved

prestist force-pushed the sgdisk-fdisk branch from bba6095 to aaff066 Compare September 16, 2024 20:40

prestist commented Sep 17, 2024

View reviewed changes

prestist force-pushed the sgdisk-fdisk branch from 034d49f to 88e9194 Compare September 20, 2024 16:52

jlebon reviewed Sep 24, 2024

View reviewed changes

prestist force-pushed the sgdisk-fdisk branch 3 times, most recently from 1a20248 to 8e7dc92 Compare October 8, 2024 21:49

prestist force-pushed the sgdisk-fdisk branch from 8e7dc92 to 74f6842 Compare October 9, 2024 15:12

prestist force-pushed the sgdisk-fdisk branch 4 times, most recently from 32b61d7 to 99e632a Compare October 16, 2024 19:59

dustymabe mentioned this pull request Mar 5, 2025

Migrate from gdisk (sgdisk) to fdisk (sfdisk) coreos/fedora-coreos-tracker#1787

Open

prestist force-pushed the sgdisk-fdisk branch 3 times, most recently from 9b43031 to d10ef64 Compare August 29, 2025 17:42

prestist force-pushed the sgdisk-fdisk branch from f35a242 to 90fb7b2 Compare September 10, 2025 13:09

prestist force-pushed the sgdisk-fdisk branch from 90fb7b2 to 9d3dfd5 Compare October 15, 2025 17:57

distro: add sfdisk cmd

902b093

prestist added 2 commits October 15, 2025 14:11

sfdisk: add sfdisk module with 'pretend' support

322626a

This is a copy of sgdisk, with mapping around sfdisk tooling. Update the buildOptions to understand sfdisk requirements and focus on adding support for the pretend function. Fixes: https://issues.redhat.com/browse/COS-2837

prestist force-pushed the sgdisk-fdisk branch 2 times, most recently from 9b2cb1d to 39132af Compare October 15, 2025 18:30

fix failing test wip

22f2822

prestist force-pushed the sgdisk-fdisk branch from 39132af to 22f2822 Compare October 15, 2025 18:39

alicefr reviewed Feb 6, 2026

View reviewed changes

Comments

Conversation

prestist commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestist commented Sep 3, 2024

Uh oh!

prestist commented Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestist commented Sep 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prestist commented Sep 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prestist commented Sep 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlebon Sep 24, 2024 • edited by travier Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prestist commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestist commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alicefr Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prestist commented Aug 26, 2024 •

edited

Loading

prestist commented Sep 4, 2024 •

edited

Loading

jlebon Sep 24, 2024 •

edited by travier

Loading

prestist commented Oct 8, 2024 •

edited

Loading

prestist commented Sep 10, 2025 •

edited

Loading

alicefr Feb 6, 2026 •

edited

Loading